lnwire+netann: update structure of g175 messages to be pure TLV #9175

ellemouton · 2024-10-11T14:42:39Z

Update the structure of all gossip 1.75 messages to be purely TLV.
With this update, the signature is also a TLV record. There is a un-signed
and signed range of TLVs. The signature along with things like SPV proofs
will fall in the un-signed range.
I've used the same un-signed range used by Bolt12 messages.

This PR introduces a new PureTLVMessage interface for messages like this
and adds a comprehensive test to ensure that old clients (who are aware of
fewer fields of a message than new clients) and new clients still generate the
same correct digest of a message regardless of if they are aware of all the
records or not.

Bonus Commit

Ok i've also now updated this PR to include validating the signature of a
ChannelAnnouncement2 message for the case where it is announcing a P2WSH channel.

ellemouton · 2024-10-16T11:26:17Z

Ok i've also now updated this PR to include validating the signature of a
ChannelAnnouncement2 message for the case where it is announcing a P2WSH channel.

go.mod

lnwire/pure_tlv.go

Roasbeef · 2024-10-16T22:44:52Z

lnwire/pure_tlv.go

+const (
+	pureTLVUnsignedRangeOneStart  = 160
+	pureTLVSignedSecondRangeStart = 1000000000
+	pureTLVUnsignedRangeTwoStart  = 3000000000


How did we arrive at these values?

Also I think we should try to capture this in the spec, independent of G 1.75.

How did we arrive at these values?

Stole from Bolt12

Also I think we should try to capture this in the spec, independent of G 1.75.

Agreed - however it is currently in bolt12. I'll update the G157 spec PR so that it moves the definition to a more "shared" location (like BOLT 1)

Roasbeef · 2024-10-16T22:46:52Z

lnwire/pure_tlv.go

+
+// SerialiseFieldsToSign serialises all the records from the given
+// PureTLVMessage that fall within the signed TLV range.
+func SerialiseFieldsToSign(msg PureTLVMessage) ([]byte, error) {


We might want to adopt a similar structure for the signed portion of the dyn commit messages @ProofOfKeags. Basically we copied over from the way BOLT 12 works, and we can use the TLV type values, then let this handle how serialization works (does require a message to be pure TLV).

Roasbeef · 2024-10-16T22:50:36Z

lnwire/pure_tlv_test.go

+	// Any extra fields in the signed range that we do not yet know about,
+	// but we need to keep them for signature validation and to produce a
+	// valid message.
+	ExtraFieldsInSignedRange map[uint64][]byte


Should something like this be split out into an embeddable struct?

updated to be an embedded struct - let me know if this is what you had in mind

Roasbeef · 2024-10-16T22:54:27Z

lnwire/announcement_signatures_2.go

+	// Any extra fields in the signed range that we do not yet know about,
+	// but we need to keep them for signature validation and to produce a
+	// valid message.
+	ExtraFieldsInSignedRange map[uint64][]byte


Also why can't we just continue to store it in the serialized form?

Feels like unnecessary duplication? All the fields we know of, we explicitly handle above. Then this only contains all the other fields in the unsigned range that we need to keep. Just feels more structured to me. This pure tlv structure with unsigned and signed ranges feels fundamentally different to me than the previous usage of TLV which is purely tacked on the the end of a message and we always needed to keep the whole thing since the signature covers everything.

Happy to discuss further!

Roasbeef · 2024-10-16T22:54:56Z

discovery/gossiper.go

+	// AuthenticatedGossiper. Any announcements that don't match this
+	// chain hash will be ignored. This is an internal config value obtained
+	// from ChainParams.
+	chainHash *chainhash.Hash


Why do we need it here if we have ChainParams?

just had it there as a helper so that we dont need to do cfg.ChainParams.GenesisHash everytime.... but happy to remove

Roasbeef · 2024-10-16T22:55:02Z

discovery/gossiper.go

-	ChainHash chainhash.Hash
+	// ChainParams holds the chain parameters for the active network this
+	// node is participating on.
+	ChainParams *chaincfg.Params


I think this can be a normal value rather than a pointer.

wont it copy over struct values then though?

Roasbeef · 2024-10-16T22:55:38Z

discovery/gossiper.go

-	[]byte, error) {
+	txscript.ScriptClass, btcutil.Address, error) {
+
+	pkScript, err := lnwallet.FetchPKScriptWithQuit(


What about the merkle proof case?

that wont be supported from the get go. That will be a separate feature bit

Roasbeef · 2024-10-16T22:55:56Z

netann/channel_announcement.go

@@ -108,7 +110,8 @@ func CreateChanAnnouncement(chanProof *models.ChannelAuthProof,

 // FetchPkScript defines a function that can be used to fetch the output script
 // for the transaction with the given SCID.
-type FetchPkScript func(*lnwire.ShortChannelID) ([]byte, error)
+type FetchPkScript func(*lnwire.ShortChannelID) (txscript.ScriptClass,


Does the arg here need to be a pointer?

else it is pass by value right?

coderabbitai · 2024-12-02T08:28:43Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

PureTLVMessage describes an LN message that is a pure TLV stream. If the message includes a signature, it will sign all the TLV records in the inclusive ranges: 0 to 159 and 1000000000 to 2999999999. A comprehensive test is added that shows how two versions of the same message remain forward compatible.

So that we can use it as a TLV record type.

Such that all fields are now TLV (including the signature).

such that all fields are now TLV including the signature.

This commit expands the ChannelAnnouncement2 validation for the case where it is announcing a P2WSH channel.

lightninglabs-deploy · 2024-12-09T10:56:34Z

@ellemouton, remember to re-request review from reviewers when ready

ellemouton self-assigned this Oct 11, 2024

ellemouton added the gossip label Oct 11, 2024

ellemouton force-pushed the g175UpdateMessageStructure branch 3 times, most recently from 4160fd5 to 5d3c965 Compare October 16, 2024 11:24

ellemouton force-pushed the g175UpdateMessageStructure branch from 5d3c965 to 6f0ce3b Compare October 16, 2024 11:39

Roasbeef requested changes Oct 16, 2024

View reviewed changes

ellemouton force-pushed the g175UpdateMessageStructure branch from 6f0ce3b to 5e509ab Compare December 2, 2024 08:28

go.mod: update TLV package dep

4d46b5c

ellemouton force-pushed the g175UpdateMessageStructure branch from 5e509ab to f9b6397 Compare December 2, 2024 09:11

ellemouton added 6 commits December 2, 2024 12:49

lnwire: let ChannelID implement RecordProducer

db76d0b

So that we can use it as a TLV record type.

lnwire+netann: update ChannelAnnouncement2 structure

1ffd3b2

Such that all fields are now TLV (including the signature).

lnwire+netann: update ChannelUpdate2

34afb59

such that all fields are now TLV including the signature.

lnwire: update AnnounceSigs2 to use pure TLV

fc40b72

netann: update ChanAnn2 validation to work for P2WSH channels

9d67b34

This commit expands the ChannelAnnouncement2 validation for the case where it is announcing a P2WSH channel.

ellemouton force-pushed the g175UpdateMessageStructure branch from f9b6397 to 72d8784 Compare December 2, 2024 10:49

lnwire: add NodeAnnouncement2

53bdf65

ellemouton force-pushed the g175UpdateMessageStructure branch from 72d8784 to 53bdf65 Compare December 2, 2024 11:25

saubyk added this to the 0.20.0 milestone Dec 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnwire+netann: update structure of g175 messages to be pure TLV #9175

lnwire+netann: update structure of g175 messages to be pure TLV #9175

ellemouton commented Oct 11, 2024 •

edited

Loading

ellemouton commented Oct 16, 2024

Roasbeef Oct 16, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

ellemouton Oct 24, 2024

Roasbeef Oct 16, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

Roasbeef Oct 16, 2024

ellemouton Dec 2, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

Roasbeef Oct 16, 2024

ellemouton Oct 24, 2024

coderabbitai bot commented Dec 2, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

lightninglabs-deploy commented Dec 9, 2024

lnwire+netann: update structure of g175 messages to be pure TLV #9175

Are you sure you want to change the base?

lnwire+netann: update structure of g175 messages to be pure TLV #9175

Conversation

ellemouton commented Oct 11, 2024 • edited Loading

Bonus Commit

ellemouton commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot commented Dec 2, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

lightninglabs-deploy commented Dec 9, 2024

ellemouton commented Oct 11, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)